Expose single command execute methods #2

bostko · 2015-10-09T14:56:50Z

@alasdairhodge , @aledsage Can you review it.
I think it is good to have such methods exposed.

I would even suggest to remove the compileScript but external projects are still dependent on it.

aledsage · 2015-10-12T11:46:32Z

src/main/java/io/cloudsoft/winrm4j/winrm/WinRmTool.java

Should we just overload the executePs method (with this one taking a string), rather than using a different name?

Same for executeCommand: I'd prefer consistency of naming. If we think that executeScript was not the best name, then we can look at changing that name.

We should include some javadoc to say what this command does (it's unfortunate that there wasn't any javadoc on the original executePs(List) method).

@aledsage
What do you thing about renaming it only for Ps, executePs(String) and executePs(List<String>).
and keep executeCommand(String) and executeCommand(List) for the "Command Prompt" commands? Really we do notexecuteScript` no script was being composed here so I think it is wrong.

Makes sense, so +1. But let's keep old names as deprecated until Brooklyn catches up.

bostko · 2015-10-12T15:18:16Z

@aledsage can you review it again

aledsage · 2015-10-12T20:19:25Z

src/main/java/io/cloudsoft/winrm4j/winrm/WinRmTool.java

This kind of thing is implementation details rather than part of the contract. I'd therefore be very hesitant about including it in the javadoc.

aledsage · 2015-12-17T12:07:51Z

src/main/java/io/cloudsoft/winrm4j/winrm/WinRmTool.java

Can you explain the reason for the method renames (rather than just overloading the existing methods that have the same behaviour - e.g. executeScript(List) is exactly like this executeCommand(String) if you pass a list of size one).

bostko · 2016-01-11T09:36:31Z

Notice it depends on apache/incubator-brooklyn#950

bostko · 2016-01-15T14:56:37Z

winrm4j/src/main/java/io/cloudsoft/winrm4j/winrm/WinRmTool.java

Although I deprecated it. @neykov has several tests for executePs(List<String>)

bostko · 2016-01-15T14:59:49Z

@neykov can you review it

neykov · 2016-01-15T15:07:11Z

@bostko I think we should keep the (List<String>) methods and just add (String) overloads (just for sugaring over the list alternative). What's the reason for removing them?
Note that the winrm service doesn't support concatenated lines with \r\n nicely - it will return the output of the first command only (see disabled tests). That means that the executeCommand(String) is really just for a single line of batch script.
Another reason for keeping them is that we can optimize and open a single shell, then execute each command from the list, then close the shell (instead of opening a shell for each command as is currently which will probably break scripts depending on environment changes).

bostko · 2016-01-16T10:33:35Z

@neykov @aledsage

I kept the old signatures.
executeScript(List) is chaining commands with & now instead of "\r\n"

neykov · 2016-01-18T07:09:36Z

winrm4j/src/test/java/io/cloudsoft/winrm4j/winrm/WinRmToolExecLiveTest.java

Wht not create analogous assertExecPs, instead of making compilePs public?

+1 will change that in my branch of this.

- moved to a working version of jaxws-maven-plugin - exit codes assertion

aledsage · 2016-01-18T16:21:01Z

Closing - I've merged #11 which includes this commit.

bostko mentioned this pull request Oct 12, 2015

WinRM commands apache/incubator-brooklyn#947

Closed

aledsage reviewed Oct 12, 2015
View reviewed changes

bostko force-pushed the single_command branch from 6921775 to caa3256 Compare October 12, 2015 15:17

bostko force-pushed the single_command branch 2 times, most recently from 625df2b to 8f8f899 Compare October 12, 2015 15:24

aledsage reviewed Oct 12, 2015
View reviewed changes

bostko mentioned this pull request Oct 12, 2015

Windows script files apache/incubator-brooklyn#950

Open

bostko force-pushed the single_command branch from 8f8f899 to 41c3073 Compare October 13, 2015 07:19

aledsage reviewed Dec 17, 2015
View reviewed changes

bostko force-pushed the single_command branch from 41c3073 to e07c19a Compare January 11, 2016 09:32

bostko force-pushed the single_command branch 8 times, most recently from a90b9c4 to fb23919 Compare January 15, 2016 14:54

bostko reviewed Jan 15, 2016
View reviewed changes

bostko force-pushed the single_command branch 3 times, most recently from 57b1dc7 to 29a5a90 Compare January 15, 2016 20:09

bostko force-pushed the single_command branch from 29a5a90 to 82c8990 Compare January 15, 2016 20:11

neykov reviewed Jan 18, 2016
View reviewed changes

bostko force-pushed the single_command branch from 82c8990 to feb2331 Compare January 18, 2016 12:35

Expose single command execute methods

10af906

- moved to a working version of jaxws-maven-plugin - exit codes assertion

bostko force-pushed the single_command branch from feb2331 to 10af906 Compare January 18, 2016 12:37

aledsage mentioned this pull request Jan 18, 2016

single command (aled) #11

Merged

aledsage closed this Jan 18, 2016

Expose single command execute methods #2

Expose single command execute methods #2

Uh oh!

Conversation

bostko commented Oct 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bostko commented Oct 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bostko commented Jan 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bostko commented Jan 15, 2016

Uh oh!

neykov commented Jan 15, 2016

Uh oh!

bostko commented Jan 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aledsage commented Jan 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants